SmbShare: Adding new resource #211

johlju · 2019-03-31T12:00:49Z

Pull Request (PR) description

This moves the resource xSmbShare to the module ComputerManagementDsc, and deprecate the xSmbShare module.

This Pull Request (PR) fixes the following issues

n/a

Task list

Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
Entry should say what was changed, and how that affects users (if applicable).
Resource documentation added/updated in README.md in the resource folder.
Resource parameter descriptions added/updated in schema.mof
and comment-based help.
Comment-based help added/updated.
Localization strings added/updated in all localization files as appropriate.
Examples appropriately added/updated.
Unit tests added/updated. See DSC Resource Testing Guidelines.
Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is

johlju · 2019-03-31T12:03:12Z

Working on integration tests next, and saw now that I missed some documentation in the resource folder, and the README.md.

@PlagueHO testing a draft PR to see if it helps the review or discussions from the community.

PlagueHO · 2019-03-31T18:36:28Z

Oh right. Doh! I guess it should be have the needs review then!

johlju · 2019-03-31T19:29:55Z

@PlagueHO The examples fails to compile in my fork because it sees multiple versions on ComputerManagementDsc. Is it something in my change that is causing that?

See test run in my forks working branch:
https://ci.appveyor.com/project/johlju/computermanagementdsc/builds/23490491?fullLog=true#L4983

PlagueHO · 2019-04-01T06:04:05Z

@johlju - I don't know if this is the cause of the issue, but you do need to rebase I think as you need to pick up the more recent changes.

I couldn't see anything in your code that looked like it would cause this problem! That is strange. Does it work locally?

PlagueHO · 2019-04-01T06:05:59Z

P.S. as soon as #195 is through I'm going to refactor this module to get rid of the harness - which will make it simpler and easier to debug issues like this.

johlju · 2019-04-01T06:08:34Z

Yep, I will rebase as soon as I get the last of the integration tests fixed. After that I take it out of draft and AppVeyor will run for the PR. We debug after that why there are two modules present on the node, if the problem is still there.

johlju · 2019-04-04T04:23:40Z

Updated to reflect new knowledge.

I have a slight problem with this resource, and it is the default behavior of the cmdlet New-SmbShare. If you would use this configuration.

SmbShare 'Integration_Test'
{
    Name         = 'MyShare'
    Path         = 'c:\Temp'
    FullAccess   = @()
    ChangeAccess = @()
    ReadAccess   = @()
    NoAccess     = @()
}

The share would be created with the group 'Everyone' having read access to the share. This is because the cmdlet New-SmbShare does this by design (can't find a way to make it stop). This leads to that Test-DscConfiguration (Test-TargetResource) will always fail since 'Everyone' will be reported as having read access and the property ReadAccess will return @('Everyone').
If you add any other user or group as having any access permission, then 'Everyone' is not added.

It would not be possible to remove the 'Everyone' access permission either, because we are only allowed to have on instance of this resource.

I see some options to solve this.

~~Make @('Everyone') the default for parameter ReadAccess so user must specify another (dummy) account to remove it.~~ This does not work since we don't want to actually add Everyone with read permission.
~~Remove the access permission 'Everyone' (if it exist) if ReadAccess equals @(), as a second step after the share has been created using New-SmbShare.~~ This does not work, because the 'Everyone' permission was removed to 'NoAccess' permission, denying everyone. The design is that at least on account or group must have permission.
Remove the *Access-parameters, and move them to a new resource SmbShareAccess which handles the permissions, and uses other cmdlets to add, remove, block and unblock access permissions.
Set the Null SID (S-1-0-0) in the NoAccess parameter when providing all empty collections.

kungfoome · 2019-04-04T05:13:28Z

I have a slight problem with this resource, and it is the default behavior of the cmdlet New-SmbShare. If you would use this configuration.
SmbShare 'Integration_Test'
{
    Name         = 'MyShare'
    Path         = 'c:\Temp'
    FullAccess   = @()
    ChangeAccess = @()
    ReadAccess   = @()
    NoAccess     = @()
}
The share would be created with the group 'Everyone' having read access to the share. This is because the cmdlet New-SmbShare does this by design (can't find a way to make it stop). If you add any other user or group as having read access, then 'Everyone' is not added.
This leads to that it is not possible to have this configuration. Because Test-DscConfiguration (Test-TargetResource) will always fail since 'Everyone' will be reported as having read access and the property ReadAccess will return @('Everyone').
SmbShare 'Integration_Test'
{
    Name         = 'MyShare'
    Path         = 'c:\Temp'
    FullAccess   = @()
    ChangeAccess = @('User1')
    ReadAccess   = @()
    NoAccess     = @()
}
It would not be possible to remove the 'Everyone' access permission either, because we are only allowed to have this configuration once.

I see some options to solve this.

Make @('Everyone') the default for parameter ReadAccess so user must specify another (dummy) account to remove it.

Remove the access permission 'Everyone' (if it exist) if ReadAccess equals @(), as a second step after the share has been created using New-SmbShare.

Remove the *Access-parameters, and move them to a new resource SmbShareAccess which handles the permissions, and uses other cmdlets to add, remove, block and unblock access permissions.

I'm leaning towards option 2.

Ok this is weird. The spec does say that default is none. If i pass in None, I get a user account None. The sid is an actual user though and it's not using a built-in sid. This led me into trying S-1-0-0, which does work! I would use that.

New-SmbShare -Name "deleteme" -Path "C:\" -ReadAccess 'NULL SID'

Name     ScopeName Path Description
----     --------- ---- -----------
deleteme *         C:\

Get-SmbShareAccess -Name "deleteme"
Name     ScopeName AccountName AccessControlType AccessRight
----     --------- ----------- ----------------- -----------
deleteme *         NULL SID    Allow             Read

johlju · 2019-04-04T08:24:11Z

Actually it seems that if removing the automatically added 'Everyone' from read access, then 'Everyone' is instead moved to NoAccess property so that Everyone in 'Denied'. It seems that by design an access rule must exist. If adding a user to any other access permission, 'Everyone' is not added.

So this is only a problem when specifying all access permission with empty collections, see below.

SmbShare 'Integration_Test'
{
    Name         = 'MyShare'
    Path         = 'c:\Temp'
    FullAccess   = @()
    ChangeAccess = @()
    ReadAccess   = @()
    NoAccess     = @()
}

@kungfu71186 thanks for finding the Null SID (S-1-0-0) idea! In the above scenario, when all collections is empty, we should add Null SID to the NoAccess parameter. So that there is no way anything get permission (if there is some way of adding a user to the Null SID "empty group"). 😄

PS> New-SmbShare -Name "deleteme" -Path "C:\Temp" -NoAccess 'NULL SID'

Name     ScopeName Path    Description
----     --------- ----    -----------
deleteme *         C:\Temp


PS> Get-SmbShareAccess -Name "deleteme"

Name     ScopeName AccountName AccessControlType AccessRight
----     --------- ----------- ----------------- -----------
deleteme *         NULL SID    Deny              Full

If it find the Null SID in the NoAccess property, the resource should ignore that access permission. Although, maybe it should remove it if there are another access permission added later in the configuration. Sort of clean up. :) It should only ignore it as long as all access parameters are empty collections.

johlju · 2019-04-04T08:31:39Z

if using this configuration, will will ignore and just assume that the user knows the default is that Everyone is added as read access permission. We should note it in the documentation though.

SmbShare 'Integration_Test'
{
    Name         = 'MyShare'
    Path         = 'c:\Temp'
}

johlju · 2019-04-04T15:37:19Z

After thinking on this during the day, I think we instead just throw an error if all of the access permission parameters are configured with empty collections. The reason is that it is not possible to modify the permissions manually if using an configuration where all the access permission parameters are empty collections, the manually added users are always removed. Instead it should throw an error saying that either remove the access permission parameters completely or add an account or group to one of the access permission parameters.

kungfoome · 2019-04-04T16:11:35Z

Seems reasonable. Honestly, this is a very special edge case that probably shouldn't be used anyway. Otherwise you can just specify absent and your problems are solved and it's more secure. I think leaving the default to everyone read access if nothing is specified is fine as well.

But you have options now :)

Maybe just leave a note in there that it's intentional to not support it as well for the reason you stated above so if anyone ever makes an issue of it, we can quickly say it's already been thought of.

johlju · 2019-04-04T16:30:15Z

@kungfu71186 That was my thought as well, if there should be no permission then it would be better to just remove the share. 😄 Yes I appreciate the help, because that was led me to this. 🙇

codecov-io · 2019-04-04T17:09:17Z

Codecov Report

Merging #211 into dev will increase coverage by 2%.
The diff coverage is 96%.

@@         Coverage Diff          @@
##            dev   #211    +/-   ##
====================================
+ Coverage    83%    85%    +2%     
====================================
  Files        10     11     +1     
  Lines      1074   1242   +168     
====================================
+ Hits        897   1064   +167     
- Misses      177    178     +1

johlju · 2019-04-04T17:17:07Z

@PlagueHO This is ready for review now. I'm debugging to find where and how it gets two instances of the module during testing.

johlju · 2019-04-04T18:02:34Z

Returning two module folder locations from Get-Module -ListAvailable

VERBOSE: C:\projects\computermanagementdsc\Modules\ComputerManagementDsc\ComputerManagementDsc.psd1
VERBOSE: C:\Windows\system32\WindowsPowerShell\v1.0\Modules\ComputerManagementDsc\ComputerManagementDsc.psd1

First one is imported here:
https://github.com/PowerShell/DscResource.Tests/blob/4c919dcda0fb1e52e645bcbdf9c40b9f9e4a43c6/TestHelper.psm1#L336

Second one is here:
https://github.com/PowerShell/DscResource.Tests/blob/4c919dcda0fb1e52e645bcbdf9c40b9f9e4a43c6/Meta.Tests.ps1#L595-L605

The problem might be that the meta tests are not run first as it is in a "non-harness modul". But why is the problem showing now. 🤔

PlagueHO · 2019-04-04T18:15:48Z

Awesome stuff @johlju - I'll review this weekend! Sorry about not commenting earlier. Day job :/

PlagueHO · 2019-04-04T18:17:17Z

That is really strange. Perhaps it is just better if I do the work to get this off Harness mode? It won't take me long to do - maybe a couple of hours?

johlju · 2019-04-04T18:22:13Z

@PlagueHO Yeah, you were planning on doing it anyway, so lets do that. But no hurry, I won't be available this weekend to fix any review comments, so take your time. 😄

I just ran the dev version, and it is passing. Maybe it is the unit test template or integration test template that messes this up. 🤔

PlagueHO · 2019-04-05T07:55:05Z

@johlju - I've completed the harness refactor. I'm just waiting to get #195 through and then I'll rebase onto dev again with the refactor and submit the PR for review.

Might need to get you to review it though 😢 It is mostly just file moves and adjustments to the test headers.

johlju · 2019-04-05T09:58:17Z

No problem, I will help review 😄

johlju

Reviewable status: 0 of 15 files reviewed, 1 unresolved discussion

Tests/Integration/MSFT_SmbShare.config.ps1, line 171 at r2 (raw file):

#ContinuouslyAvailable = $true

I missed this! I need to create a virtual drive I think so we can test till property. Forgot to look into it.
https://blog.workinghardinit.work/tag/continuously-available-file-shares/

johlju

Reviewable status: 0 of 15 files reviewed, 3 unresolved discussions

Modules/ComputerManagementDsc/DSCResources/MSFT_SmbShare/MSFT_SmbShare.psm1, line 77 at r2 (raw file):

$returnValue['FolderEnumerationMode'] = $smbShare.FolderEnumerationMode

This returns the wrong type.

VERBOSE: [APPVYR-WIN]:                            [[SmbShare]Integration_Test] NOTMATCH: Type mismatch for property 'FolderEnumerationMode' Current state type is 'FolderEnumerationMode' and desired type is 'String'.

https://ci.appveyor.com/project/PowerShell/computermanagementdsc/builds/23632778?fullLog=true#L3772

Modules/ComputerManagementDsc/DSCResources/MSFT_SmbShare/MSFT_SmbShare.psm1, line 78 at r2 (raw file):

$returnValue['CachingMode'] = $smbShare.CachingMode

This returns the wrong type.

VERBOSE: [APPVYR-WIN]:                            [[SmbShare]Integration_Test] NOTMATCH: Type mismatch for property 'CachingMode' Current state type is 'CachingMode' and desired type is 'String'.

https://ci.appveyor.com/project/PowerShell/computermanagementdsc/builds/23632778?fullLog=true#L3776

johlju · 2019-04-12T13:47:06Z

I will look into getting the missed integration tests into this PR this weekend, and also rebase when the refactor PR is merged.

johlju · 2019-06-30T12:59:15Z

@PlagueHO rebased 😄

PlagueHO · 2019-07-01T09:55:24Z

Time to review! 😁

PlagueHO

Looking good @johlju - just some minor tweaks and we can merge! Woo!

Reviewed 1 of 2 files at r4, 5 of 12 files at r5.
Reviewable status: 8 of 15 files reviewed, 33 unresolved discussions (waiting on @johlju and @PlagueHO)

CHANGELOG.md, line 5 at r5 (raw file):

## Unreleased

- xComputer:

Just noticed this is incorrect - should be Computer - not xComputer - can you correct it?

CHANGELOG.md, line 8 at r5 (raw file):

  - Fix for 'directory service is busy' error when joining a domain and renaming
    a computer when JoinOU is specified - Fixes [Issue #221](https://github.com/PowerShell/ComputerManagementDsc/issues/221).
- Added new resource

Do we want to put the name of the resource we are adding on this line? E.g.

- Added new resource SmbShare
  - Moved and improved from deprecated module xSmbShare.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 123 at r5 (raw file):

                }

                It 'Should mock call to Get-SmbShare and return membership' {

It doesn't look like we're actually checking that the mock is called until the next assertion. So should this just say "Should return the correct values"?

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 141 at r5 (raw file):

                It 'Should call the mock function Get-SmbShare' {
                    $getTargetResourceResult = Get-TargetResource @testParameters

Do we need to recall Get-TargetResource here?

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 146 at r5 (raw file):

                It 'Should Call the mock function Get-SmbShareAccess' {
                    $getTargetResourceResult = Get-TargetResource @testParameters

Do we need to recall Get-TargetResource here?

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 183 at r5 (raw file):

                    $getTargetResourceResult.NoAccess | Should -HaveCount 0
                }

Should we also assert that Get-SmbShare was called?

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 223 at r5 (raw file):

                    Context 'When no access permission is given' {
                        It 'Should call the correct mocks' {

I can't actually see any Assert-MockCalled in this it block - shouldn't there be based on the description?

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 350 at r5 (raw file):

                    }
                }

Remove extra blank line.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 356 at r5 (raw file):

        Describe 'MSFT_SmbShare\Test-TargetResource' -Tag 'Test' {
            Context 'When the system is not in the desired state' {
                Context 'When no members in provided in neither access permission collection' {

This context doesn't seem to make sense? Could it be reworded? "When no member are provided in any of the access permission collections"?

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 683 at r5 (raw file):

                    }

                    It 'Should call the correct mock' {

mock->mocks - also, it should not throw an exception.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 709 at r5 (raw file):

                    }

                    It 'Should call the correct mock' {

mock->mocks - also, it should not throw an exception.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 735 at r5 (raw file):

                    }

                    It 'Should call the correct mock' {

mock->mocks - also, it should not throw an exception.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 767 at r5 (raw file):

                    }

                    It 'Should call the correct mock' {

mock->mocks - also, it should not throw an exception.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 812 at r5 (raw file):

                    }

                    It 'Should call the correct mock' {

mock->mocks - also, it should not throw an exception.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 863 at r5 (raw file):

                    }

                    It 'Should call the correct mock' {

mock->mocks - also, it should not throw an exception.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 890 at r5 (raw file):

                    }

                    It 'Should call the correct mocks' {

Also, it should not throw an exception.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 921 at r5 (raw file):

                    }

                    It 'Should call the correct mock' {

mock->mocks - also, it should not throw an exception.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 947 at r5 (raw file):

                    }

                    It 'Should call the correct mock' {

mock->mocks - also, it should not throw an exception.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 975 at r5 (raw file):

                    }

                    It 'Should call the correct mock' {

mock->mocks - also, it should not throw an exception.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 1018 at r5 (raw file):

                    }

                    It 'Should call the correct mock' {

mock->mocks - also, it should not throw an exception.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 1095 at r5 (raw file):

                }

                Context 'When providing no member in either of the access permission collections' {

Could read - When not providing any members in any of the access permission collections"

DSCResources/MSFT_SmbShare/MSFT_SmbShare.psm1, line 304 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Done. This is already supported. The helper function Remove-SmbShareAccessPermission will only remove accounts that should not have permission (it does not remove all permissions, at least that was my intention, and I think it is coded that way). 🤔
Maybe the naming of the functions are less then good. Maybe we should add both the Remove- and Add- to a Set- instead? If so I could add an issue to track that.

Let's see how it goes. I think it should be OK to leave as is for now - because we're not going to be continuously applying set.

DSCResources/MSFT_SmbShare/MSFT_SmbShare.psm1, line 549 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Done. I think I overdid the comments in this code just to make sure I remembered all the stuff it should do, or did. 🙂

I completely understand and do the same thing 😁

DSCResources/MSFT_SmbShare/MSFT_SmbShare.psm1, line 675 at r4 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Done. Added an issue to track this instead, you could label it as 'good first issue' too. 😄 Did this so we can get this one through.

Sounds good!

DSCResources/MSFT_SmbShare/MSFT_SmbShare.psm1, line 273 at r5 (raw file):

                }

            $parametersToRemove | ForEach-Object {

Should have -Process parameter.

johlju

I think I got them all! 😄

Reviewable status: 4 of 17 files reviewed, 33 unresolved discussions (waiting on @PlagueHO)

CHANGELOG.md, line 5 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Just noticed this is incorrect - should be Computer - not xComputer - can you correct it?

Done.

CHANGELOG.md, line 8 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Do we want to put the name of the resource we are adding on this line? E.g.
- Added new resource SmbShare
  - Moved and improved from deprecated module xSmbShare.

Done.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 123 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

It doesn't look like we're actually checking that the mock is called until the next assertion. So should this just say "Should return the correct values"?

Done.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 141 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Do we need to recall Get-TargetResource here?

Done. If there is a seperate It-block I my opinion the call to the function being tested should always be called in the It-block (instead of using -Scope Context). In this case I think that was left overs from the refactored test, I normally don't do that, so I just moved the the asserts up to the first It-block. No need to have separate It-blocks for asserting the calls to the mocks. 🤔

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 146 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Do we need to recall Get-TargetResource here?

Done.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 183 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should we also assert that Get-SmbShare was called?

Done. I added it. I think I left it out since the first context block did that test, so I think left it out to speed up the tests. :)

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 223 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I can't actually see any Assert-MockCalled in this it block - shouldn't there be based on the description?

Done.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 350 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Remove extra blank line.

Done.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 356 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This context doesn't seem to make sense? Could it be reworded? "When no member are provided in any of the access permission collections"?

Done. Thanks for this. 😃 I did not understand that either, sometimes it feels like I don't even read back what I wrote 😆

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 683 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

mock->mocks - also, it should not throw an exception.

Done.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 709 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

mock->mocks - also, it should not throw an exception.

Done.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 735 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

mock->mocks - also, it should not throw an exception.

Done.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 767 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

mock->mocks - also, it should not throw an exception.

Done.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 812 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

mock->mocks - also, it should not throw an exception.

Done.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 863 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

mock->mocks - also, it should not throw an exception.

Done.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 890 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Also, it should not throw an exception.

Done.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 921 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

mock->mocks - also, it should not throw an exception.

Done.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 947 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

mock->mocks - also, it should not throw an exception.

Done.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 975 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

mock->mocks - also, it should not throw an exception.

Done.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 1018 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

mock->mocks - also, it should not throw an exception.

Done.

Tests/Unit/MSFT_SmbShare.Tests.ps1, line 1095 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Could read - When not providing any members in any of the access permission collections"

Done. Read much better, thanks! 😄

DSCResources/MSFT_SmbShare/MSFT_SmbShare.psm1, line 273 at r5 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

Should have -Process parameter.

Done. Throughout the repo. Fixed the -FilterScript throughout the repo too.

johlju · 2019-07-03T08:27:44Z

@PlagueHO Hope that was it! Ready for review again. 😃

PlagueHO · 2019-07-03T19:42:40Z

Awesome @johlju -will get onto it tonight.

PlagueHO

Awesome job @johlju - just one FYI and we're good to go.

Reviewed 7 of 12 files at r5, 6 of 6 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved

DSCResources/MSFT_WindowsEventLog/MSFT_WindowsEventLog.psm1, line 180 at r6 (raw file):

    if ($PSBoundParameters.ContainsKey('LogRetentionDays'))
    {
        if ($LogMode -eq 'AutoBackup' -and (Get-EventLog -List | Where-Object -FilterScript {$_.Log -like $LogName}))

FYI, I know this is existing code, but maybe we could make a change to improve this code:

Why not assign the (Get-EventLog -List | Where-Object -FilterScript { $_.Log -like $LogName }) to a variable before the if block and then you can reuse it within the if block and won't have to run the same expression again. This would improve performance.

johlju

Reviewable status: complete! all files reviewed, all discussions resolved

DSCResources/MSFT_WindowsEventLog/MSFT_WindowsEventLog.psm1, line 180 at r6 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

FYI, I know this is existing code, but maybe we could make a change to improve this code:

Why not assign the (Get-EventLog -List | Where-Object -FilterScript { $_.Log -like $LogName }) to a variable before the if block and then you can reuse it within the if block and won't have to run the same expression again. This would improve performance.

Created issue for this since I only did a style change (already outside the scope of the PR), and the two uses different evaluations, one uses -like and one uses -eq. Fixing that means potentially changing tests, and could change functionality, meaning I need to dig deeper to resolve, that is outside the scope of this PR. 🙂 Let's make that issue a 'good first issue' so someone new in the community can contribute to this.

PlagueHO

Reviewable status: complete! all files reviewed, all discussions resolved

DSCResources/MSFT_WindowsEventLog/MSFT_WindowsEventLog.psm1, line 180 at r6 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Created issue for this since I only did a style change (already outside the scope of the PR), and the two uses different evaluations, one uses -like and one uses -eq. Fixing that means potentially changing tests, and could change functionality, meaning I need to dig deeper to resolve, that is outside the scope of this PR. 🙂 Let's make that issue a 'good first issue' so someone new in the community can contribute to this.

Ah yes, you're right. I think I recall seeing that at the time when I first reviewed this and decided not to address it yet. So good idea about issue.

PlagueHO

Reviewable status: complete! all files reviewed, all discussions resolved

johlju mentioned this pull request Mar 31, 2019

xSmbShare resource does not take into account properties other than Name, Path, Ensure when doing a Test-TargetResource dsccommunity/xSmbShare#2

Closed

PlagueHO added the needs review The pull request needs a code review. label Mar 31, 2019

PlagueHO removed the needs review The pull request needs a code review. label Mar 31, 2019

johlju marked this pull request as ready for review April 4, 2019 17:01

johlju force-pushed the add-smbshare branch from 5ba83db to 666703e Compare April 4, 2019 17:03

johlju commented Apr 5, 2019

View reviewed changes

johlju mentioned this pull request Apr 10, 2019

Permission change doesn't take effect dsccommunity/xSmbShare#25

Closed

johlju added 17 commits June 30, 2019 14:54

Fix correct type on read properties

e1e288a

Revert change in common module

c73a48c

Remove ValuesToCheck to just check desired properties

9acd315

Add missing code

a107bb3

Fix return types in Get-TargetResource

fe58508

Add note about the property ContinuouslyAvailable

89f53ca

Update README.md

a3cc36c

Move files after project model refactor

6fc6e70

Fix correct template in integration test

2f78d0e

Consistently using ToString()

1c34169

Fix mock in tests

932396e

Update CHANGELOG.md

16a8c08

FIx review comments at r4

b4c557c

Fix bug in new code

0c9cda0

Fix variable

d016483

Fix examples

667b707

Fix more typos in examples

808c77b

johlju force-pushed the add-smbshare branch from f1784ce to 808c77b Compare June 30, 2019 12:56

PlagueHO requested changes Jul 1, 2019

View reviewed changes

Fix review comments at r5

6ed69fe

johlju commented Jul 3, 2019

View reviewed changes

PlagueHO approved these changes Jul 4, 2019

View reviewed changes

johlju commented Jul 4, 2019

View reviewed changes

PlagueHO approved these changes Jul 4, 2019

View reviewed changes

PlagueHO merged commit 5b12118 into dsccommunity:dev Jul 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SmbShare: Adding new resource #211

SmbShare: Adding new resource #211

johlju commented Mar 31, 2019 •

edited

Loading

johlju commented Mar 31, 2019

PlagueHO commented Mar 31, 2019

johlju commented Mar 31, 2019

PlagueHO commented Apr 1, 2019

PlagueHO commented Apr 1, 2019

johlju commented Apr 1, 2019

johlju commented Apr 4, 2019 •

edited

Loading

kungfoome commented Apr 4, 2019

johlju commented Apr 4, 2019 •

edited

Loading

johlju commented Apr 4, 2019

johlju commented Apr 4, 2019

kungfoome commented Apr 4, 2019 •

edited

Loading

johlju commented Apr 4, 2019

codecov-io commented Apr 4, 2019 •

edited

Loading

johlju commented Apr 4, 2019 •

edited

Loading

johlju commented Apr 4, 2019

PlagueHO commented Apr 4, 2019

PlagueHO commented Apr 4, 2019

johlju commented Apr 4, 2019

PlagueHO commented Apr 5, 2019

johlju commented Apr 5, 2019

johlju left a comment

johlju left a comment

johlju commented Apr 12, 2019

johlju commented Jun 30, 2019

PlagueHO commented Jul 1, 2019

PlagueHO left a comment

johlju left a comment

johlju commented Jul 3, 2019

PlagueHO commented Jul 3, 2019

PlagueHO left a comment

johlju left a comment

PlagueHO left a comment

PlagueHO left a comment

SmbShare: Adding new resource #211

SmbShare: Adding new resource #211

Conversation

johlju commented Mar 31, 2019 • edited Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

johlju commented Mar 31, 2019

PlagueHO commented Mar 31, 2019

johlju commented Mar 31, 2019

PlagueHO commented Apr 1, 2019

PlagueHO commented Apr 1, 2019

johlju commented Apr 1, 2019

johlju commented Apr 4, 2019 • edited Loading

kungfoome commented Apr 4, 2019

johlju commented Apr 4, 2019 • edited Loading

johlju commented Apr 4, 2019

johlju commented Apr 4, 2019

kungfoome commented Apr 4, 2019 • edited Loading

johlju commented Apr 4, 2019

codecov-io commented Apr 4, 2019 • edited Loading

Codecov Report

johlju commented Apr 4, 2019 • edited Loading

johlju commented Apr 4, 2019

PlagueHO commented Apr 4, 2019

PlagueHO commented Apr 4, 2019

johlju commented Apr 4, 2019

PlagueHO commented Apr 5, 2019

johlju commented Apr 5, 2019

johlju left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

johlju commented Apr 12, 2019

johlju commented Jun 30, 2019

PlagueHO commented Jul 1, 2019

PlagueHO left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

johlju commented Jul 3, 2019

PlagueHO commented Jul 3, 2019

PlagueHO left a comment

Choose a reason for hiding this comment

johlju left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

PlagueHO left a comment

Choose a reason for hiding this comment

johlju commented Mar 31, 2019 •

edited

Loading

johlju commented Apr 4, 2019 •

edited

Loading

johlju commented Apr 4, 2019 •

edited

Loading

kungfoome commented Apr 4, 2019 •

edited

Loading

codecov-io commented Apr 4, 2019 •

edited

Loading

johlju commented Apr 4, 2019 •

edited

Loading